[BD-46] refactor: replaced legacy checkboxes #2555

PKulkoRaccoonGang · 2023-08-16T18:09:51Z

Description

replaced legacy checkboxes inside DataTable and TableFilters components

Deploy Preview

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

openedx-webhooks · 2023-08-16T18:09:55Z

Thanks for the pull request, @PKulkoRaccoonGang!

When this pull request is ready, tag your edX technical lead.

netlify · 2023-08-16T18:10:01Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`8d61d59`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/64df8b59294aad000857b70e
😎 Deploy Preview	https://deploy-preview-2555--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2023-08-16T18:34:21Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (40dd412) 91.65% compared to head (8d61d59) 91.69%.
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2555      +/-   ##
==========================================
+ Coverage   91.65%   91.69%   +0.03%     
==========================================
  Files         236      235       -1     
  Lines        4195     4214      +19     
  Branches     1012     1021       +9     
==========================================
+ Hits         3845     3864      +19     
  Misses        346      346              
  Partials        4        4

Files Changed	Coverage Δ
src/DataTable/filters/CheckboxFilter.jsx	`100.00% <100.00%> (ø)`
...rc/DataTable/filters/MultiSelectDropdownFilter.jsx	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/DataTable/filters/CheckboxFilter.jsx

src/DataTable/filters/MultiSelectDropdownFilter.jsx

adamstankiewicz · 2023-08-18T11:29:41Z

src/DataTable/filters/CheckboxFilter.jsx

+            onChange={() => changeCheckbox(value)}
+            aria-label={name}
+          >
+            {name} {number !== undefined && <Badge className="ml-2" variant="light">{number}</Badge>}


We should avoid using CSS utility classes in Paragon components as they include !important, which makes theming/branding difficult for consumers. Consider defining a new .pgn__ namespaced class name or perhaps using <Stack direction="horizontal" gap={2}> here would make sense?

Yes, you are right
I will fix it

adamstankiewicz · 2023-08-18T11:31:30Z

src/DataTable/filters/MultiSelectDropdownFilter.jsx

  return (
    <DropdownButton variant="outline-primary" id={ariaLabel.current} title={Header}>
-      <div role="group" aria-label={Header} className="pgn__dropdown-filter-checkbox-group">


[clarification] Is it intentional to remove the role="group" attribute here?

Yes, Form.CheckboxSet have role ="group" by default

Thanks for confirming! 🎉

adamstankiewicz · 2023-08-18T11:32:20Z

src/DataTable/filters/MultiSelectDropdownFilter.jsx

+            onChange={() => changeCheckbox(value)}
+            aria-label={name}
+          >
+            {name} {number && <Badge className="ml-2" variant="light">{number}</Badge>}


Similar comment as above regarding using CSS utility classes.

adamstankiewicz · 2023-08-18T11:33:05Z

src/hooks/useArrowKeyNavigation.mdx

@@ -64,7 +64,7 @@ the `arrowUp` and `arrowLeft` keys can be ignored for convenient editing of the
          <Form.Control type="password" placeholder="Password" />
        </Form.Group>
        <Form.Group>
-          <Form.Check type="checkbox" label="Confirm the entered data" />
+          <Form.Checkbox>Confirm the entered data</Form.Checkbox>


Good catch!

openedx-webhooks · 2023-09-01T12:24:40Z

@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-semantic-release · 2023-09-01T12:39:17Z

🎉 This PR is included in version 21.1.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

edx-semantic-release · 2023-09-22T13:22:47Z

🎉 This PR is included in version 22.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Aug 16, 2023

PKulkoRaccoonGang changed the title ~~[BD-46] refactor: swaped legacy checkboxes inside DataTable, TableFilters com…~~ [BD-46] refactor: swaped legacy checkboxes inside DataTable and TableFilters Aug 16, 2023

PKulkoRaccoonGang force-pushed the Peter_Kulko/legacy-checkboxes branch from b95bdf8 to 480ea47 Compare August 16, 2023 18:15

PKulkoRaccoonGang changed the title ~~[BD-46] refactor: swaped legacy checkboxes inside DataTable and TableFilters~~ [BD-46] refactor: replaced legacy checkboxes inside DataTable and TableFilters Aug 16, 2023

PKulkoRaccoonGang changed the title ~~[BD-46] refactor: replaced legacy checkboxes inside DataTable and TableFilters~~ [BD-46] refactor: replaced legacy checkboxes Aug 16, 2023

PKulkoRaccoonGang force-pushed the Peter_Kulko/legacy-checkboxes branch from 480ea47 to 1d4bcd6 Compare August 16, 2023 18:17

PKulkoRaccoonGang requested review from monteri and adamstankiewicz August 16, 2023 18:18

PKulkoRaccoonGang self-assigned this Aug 16, 2023

PKulkoRaccoonGang added the raccoon-gang label Aug 16, 2023

PKulkoRaccoonGang mentioned this pull request Aug 16, 2023

Legacy checkboxes inside a DataTable component #2519

Closed

refactor: replaced legacy checkboxes

e7165c7

PKulkoRaccoonGang force-pushed the Peter_Kulko/legacy-checkboxes branch from 1d4bcd6 to e7165c7 Compare August 16, 2023 18:41

monteri reviewed Aug 17, 2023

View reviewed changes

src/DataTable/filters/CheckboxFilter.jsx Outdated Show resolved Hide resolved

src/DataTable/filters/MultiSelectDropdownFilter.jsx Outdated Show resolved Hide resolved

refactor: refactoring after review

d67f1c6

PKulkoRaccoonGang requested a review from monteri August 17, 2023 08:35

adamstankiewicz reviewed Aug 18, 2023

View reviewed changes

refactor: refactoring after review

8d61d59

PKulkoRaccoonGang requested a review from adamstankiewicz August 18, 2023 15:48

adamstankiewicz approved these changes Sep 1, 2023

View reviewed changes

adamstankiewicz merged commit d07827d into openedx:master Sep 1, 2023
9 checks passed

edx-semantic-release added the released label Sep 1, 2023

adamstankiewicz mentioned this pull request Sep 1, 2023

DataTable filter checkboxes are slightly off center vertically #2589

Closed

edx-semantic-release added the released on @alpha label Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-46] refactor: replaced legacy checkboxes #2555

[BD-46] refactor: replaced legacy checkboxes #2555

PKulkoRaccoonGang commented Aug 16, 2023 •

edited

Loading

openedx-webhooks commented Aug 16, 2023

netlify bot commented Aug 16, 2023 •

edited

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading

adamstankiewicz Aug 18, 2023

PKulkoRaccoonGang Aug 18, 2023

adamstankiewicz Aug 18, 2023

PKulkoRaccoonGang Aug 18, 2023 •

edited

Loading

adamstankiewicz Aug 18, 2023

adamstankiewicz Aug 18, 2023

adamstankiewicz Aug 18, 2023

PKulkoRaccoonGang Aug 18, 2023

openedx-webhooks commented Sep 1, 2023

edx-semantic-release commented Sep 1, 2023

edx-semantic-release commented Sep 22, 2023

[BD-46] refactor: replaced legacy checkboxes #2555

[BD-46] refactor: replaced legacy checkboxes #2555

Conversation

PKulkoRaccoonGang commented Aug 16, 2023 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Aug 16, 2023

netlify bot commented Aug 16, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

adamstankiewicz Aug 18, 2023

Choose a reason for hiding this comment

PKulkoRaccoonGang Aug 18, 2023

Choose a reason for hiding this comment

adamstankiewicz Aug 18, 2023

Choose a reason for hiding this comment

PKulkoRaccoonGang Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Aug 18, 2023

Choose a reason for hiding this comment

adamstankiewicz Aug 18, 2023

Choose a reason for hiding this comment

adamstankiewicz Aug 18, 2023

Choose a reason for hiding this comment

PKulkoRaccoonGang Aug 18, 2023

Choose a reason for hiding this comment

openedx-webhooks commented Sep 1, 2023

edx-semantic-release commented Sep 1, 2023

edx-semantic-release commented Sep 22, 2023

PKulkoRaccoonGang commented Aug 16, 2023 •

edited

Loading

netlify bot commented Aug 16, 2023 •

edited

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading

PKulkoRaccoonGang Aug 18, 2023 •

edited

Loading